Scan multipart boundaries with Boyer-Moore-Horspool - #39

Build & Tests

carp -x test/http.carp at 3d96de5 on this armhf Pi — 479 passed, 0 failed,
exit code read from the unpiped command. carp-fmt --check clean over
http.carp, test/http.carp and gendocs.carp; angler clean over the same
three (using a binary built from angler#30's branch, so the new
byte-offset-as-char-index rule is included — the two open PRs do not collide).
CI green on both legs, verified through check-runs at 3d96de5 itself. Working
tree clean, branch sits on b7e018b which is origin/main's head. No CHANGELOG
in this repo, so nothing to file.

Multipart.at?, index-from and skip-table are all private/hidden and the
grep confirms nothing outside http.carp names them; Multipart.parse's
signature is untouched, so web.carp:645 is unaffected.

Randomised differential, 40 000 bodies, 0 differences. I built the pre-PR
at? / index-from / parse into the same binary as the new ones (reopening
Multipart so the old copy reaches the private parse-part) and compared part
count plus a digest of every part's name, filename, content-type and body. The
generator builds a well-formed 1-3 part body and then applies 0-3 mutations
(truncate, splice in a fragment, delete a byte); part bodies draw on \r\n,
--, bare \r, bare \n, --b--, and the high bytes \x80, \xff,
\xc3\xa4; the boundary is drawn from ten spellings including -, --, bb,
a-b and LongBoundary8.

cases=40000  errors=1934  diffs=0

Only 4.8 % error out, so 38 066 of those cases are two real parses compared
against each other rather than two identical failures — I re-ran the generator
after a first version came out 93 % errors and was comparing almost nothing.

The harness has teeth. Five mutants of the new scan, all killed:

The fourth is worth a sentence: including the needle's last byte in the table
sets its shift to 0, so index-from stops advancing. (for [i 0 last]) is
load-bearing, not stylistic.

The bad-character index is in range for any byte. Char is uint32_t
(core/core.h:23) and String_char_MINUS_at returns (uint8_t)(*s)[i]
(core/carp_string.h:135), so (Char.to-int (String.char-at …)) is 0-255 on
every platform regardless of whether the C char is signed. A binary body or a
non-ASCII boundary cannot walk Array.unsafe-nth off the 256-entry table.

The performance claim reproduces, and the "before" column reproduces to the
millisecond. Both parsers in one binary, two-part application/octet-stream
bodies:

11.29 -> 158.09 -> 2 458.81 is 4x the bytes for ~16x the time, so the quadratic
shape is measured, not inferred, and it lands within 1 % of the table's 11.3 /
158.5 / 2 486.

Small bodies do not regress, which is the case a 256-entry table per parse
could plausibly have cost — the common <form> post is a few hundred bytes.
200 iterations each, per-parse:

418 B    new 147 us   old 174 us
554 B    new 148 us   old 202 us
866 B    new 148 us   old 291 us
2402 B   new 155 us   old 1099 us

Faster at every size I could generate; the table build disappears into
parse-part.

Findings

1. The scan's end bound is the one thing the new tests do not pin

(<= (+ i nlen) slen) in index-from is what lets a delimiter that ends exactly
at the end of the body be found. Change it to < and the shipped suite does not
notice:

$ # (while (and (< res 0) (<= (+ i nlen) slen))  ->  (< (+ i nlen) slen)
$ carp -x test/http.carp
        Passed: 479     Failed: 0        (rc 0)

It is not an equivalent mutant — the differential above puts it at 1 830
differing bodies. The falsifying shape is a body whose final delimiter is the
last thing in it, which is what a truncated upload looks like:

baseline   "--b\r\nContent-Disposition: form-data; name=\"a\"\r\n\r\nhello\r\n--b"  ->  1 part, "hello"
<= -> <    same body                                                                ->  0 parts

The PR already knows this: the teeth-check section names boundary-at-end as
the single corpus case that catches this exact off-by-one. That corpus is not
shipped, and none of the three tests that were added covers it — they pin the
non-UTF-8 part body, the non-UTF-8 preamble and the repeated-last-byte skip
table, all of which survive the change. One assert-equal next to the other
three closes it:

  (assert-equal test
    "hello"
    &(mp-body
      &(mp-parts
        "--b\r\nContent-Disposition: form-data; name=\"a\"\r\n\r\nhello\r\n--b"
        "b")
      0)
    "a delimiter that ends at the end of the body is still found")

I checked it: 480 passed, 0 failed on this branch, and under the mutant the
run aborts on Array_unsafe_nth's n < a.len — the same way every other
mp-body test in this block dies when a part goes missing, and the same way
the skip-table mutant above dies.

Also checked, nothing found

• The two scans agree on the opening delimiter. String.index-of-string was
  strstr and the replacement scans to len, which is strlen — both stop at
  the same NUL, so swapping them changes nothing, including on the preamble path
  the body calls out.
• Skip values are never zero, so index-from always advances: the table is
  built over [0, nlen-2], giving shifts in [1, nlen-1], and the default is
  nlen. An empty boundary still gives nlen = 4 from "\r\n--".
• skip is bound before open uses it. Carp's let is sequential — I
  confirmed it rather than assuming ((let [i 7] (let [a (Int.* i 10) i 0 b (Int.* i 10)] …))
  gives a=70 b=0).
• at?'s new slen parameter is passed the same value everywhere
  (len/slen), and recomputing (String.length s) inside it instead is an
  equivalent mutant — 479/0, as it should be, since it only undoes the speedup.
• The NUL truncation is honestly scoped. It predates this branch, both scans
  hit it identically, the PR does not claim to fix it, and it is filed as #40
  rather than smuggled in. Right call.

Verdict: revise

The rewrite is correct and the numbers are real: 38 066 parsed bodies compared
against the old scan byte for byte with zero differences, five mutants killed,
the quadratic "before" column reproduced to within 1 %, and no regression on the
small bodies that a per-parse table could have cost. What is missing is one
line of test: the scan's <= end bound — the one boundary the PR's own
teeth-check singled out — is the only part of the new code the shipped suite
cannot tell from a broken version.

Build & Tests

carp -x test/http.carp at 57bb789 on this armhf Pi — 480 passed, 0 failed,
exit code read from the unpiped command, up from the 479 I measured at 3d96de5.
CI green on both legs. carp-fmt --check clean over http.carp,
test/http.carp and gendocs.carp, using a binary built fresh from carp-fmt
5e1a550 rather than a stale local one; angler clean over the same three,
using a binary built from angler#30's branch so the new
byte-offset-as-char-index rule is included — the two open PRs do not collide.
carp -x gendocs.carp leaves the tree clean. The branch sits on b7e018b,
origin/master's head, with nothing to rebase over. No CHANGELOG in this repo.

The commit is what it says: git diff 3d96de5..57bb789 is 8 added lines in
test/http.carp and nothing else, so nothing reviewed last round re-opens.

Prior feedback

The round-1 finding is closed. I measured it by building each mutant against
both suites rather than reading the new assertion:

rc 134 is Array_unsafe_nth's n < a.len assertion, at
main.c:15898: FormPart *Array_unsafe_nth__FormPart.

So both halves of the follow-up hold: the bound I named was genuinely
uncovered, and the correction — that at?'s bound was not covered either —
is right. One assertion pins two bounds for exactly the reason given: >=
makes at? reject a sub that ends at slen, which is why the trailing
--b stops matching for the same reason the < bound stops looking at it.

Findings

None. I went looking rather than taking the round-1 differential's word:

• The end bound is the only thing that moved. The rest of the scan is
  unchanged since the commit I reviewed, and the new case is additive — it does
  not weaken or replace any existing assertion in the mp-body block.
• The new assertion is not vacuous. mp-parts swallows a parse error into
  [] and mp-body reaches through Array.unsafe-nth, so a regression aborts
  the process rather than reporting a soft failure. That is loud, and it is what
  every other mp-body test in the block already does — worth knowing only
  because an abort costs you the rest of the run, not because this case
  introduced it.
• Multipart.at?, index-from and skip-table are still private/hidden
  and nothing outside http.carp names them; Multipart.parse's signature is
  untouched, so web.carp:645 is unaffected.
• The two bounds the follow-up did not mutate are genuinely unreachable or
  already covered: at?'s (< pos 0) cannot be reached — parse enters at
  open >= dashlen and advances by next + cdlen, and index-from only calls
  at? with i >= from >= 0 — and skip-table's (for [i 0 last]) was shown
  last round to hang under mutation rather than fail.

Verdict: merge

Test-only, one assertion, and it does exactly what it was asked to do: the two
bounds that survived the shipped suite at 479/0 both die at 57bb789, measured
against the round-1 suite as a control. Suite 479 -> 480, CI green, source
byte-identical to the commit reviewed last round.